Skip to content

src: refactor Environment::GetCurrent() usage#22819

Closed
addaleax wants to merge 3 commits intonodejs:masterfrom
addaleax:env-get-current
Closed

src: refactor Environment::GetCurrent() usage#22819
addaleax wants to merge 3 commits intonodejs:masterfrom
addaleax:env-get-current

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Sep 12, 2018

Make Environment::GetCurrent() return nullptr if the current
Context is not a Node.js context, and for the relevant usage of
this function, either:

  • Switch to the better GetCurrent(args) variant
  • Turn functions in to no-ops where it makes sense
  • Make it a CHECK, i.e. an API requirement, where it make sense
  • Leave a TODO comment for verifying what, if anything, is to be done
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@addaleax
src: refactor Environment::GetCurrent() usage
ba49760 
Make `Environment::GetCurrent()` return `nullptr` if the current
`Context` is not a Node.js context, and for the relevant usage of
this function, either:

- Switch to the better `GetCurrent(args)` variant
- Turn functions in to no-ops where it makes sense
- Make it a `CHECK`, i.e. an API requirement, where it make sense
- Leave a `TODO` comment for verifying what, if anything, is to be done
@addaleax addaleax added the embedding Issues and PRs related to embedding Node.js in another project. label Sep 12, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 12, 2018
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 12, 2018

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/862/pipeline

@addaleax addaleax mentioned this pull request Sep 12, 2018
Merged
4 tasks
gireeshpunathil
gireeshpunathil reviewed Sep 12, 2018
View reviewed changes
@addaleax
Copy link
Member Author

addaleax commented Sep 13, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17165/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2018
@mhdawson
Copy link
Member

mhdawson commented Sep 13, 2018

Just wondering what the plan is on the TODO's. Is it that you'll be looking at each one of them afterwards and submitting additional PRs or that if the CHECK's don't result in any failures/problems after a while they would be removed?

@addaleax
Copy link
Member Author

addaleax commented Sep 13, 2018

@mhdawson So … the issue is that each of those requires some thought about how to best address embedder/addon use cases, or how to add wiring so that it won’t be necessary. In particular:

For the node_buffer.cc ones, we could either make it an API requirement to have entered a Node.js context before making these calls (i.e. leave the checks in place), or we could do something that is closer to the author’s intention but may or may not quite be what they expect (e.g. return a Uint8Array with an unmodified prototype).

In the module_wrap cases, I just don’t know the answer (yet?) without reading up more on the possible cases that could occur here, and for the N-API ones it’s also a bit of an issue that there doesn’t quite seem to be any clear definition of what exactly a napi_env conceptually is (I feel like we might want to point to a Node.js Environment rather than an Isolate/Context pair).

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Sep 14, 2018 •
edited
Loading

@addaleax a napi_env is wherein all napi_values live. That is, if I have two napi_envs and I try to manipulate a napi_value coming from one napi_env with the other napi_env It'll crash.

In practice, we store the napi_env on the global object, so it's one napi_env per global object. We retrieve the global object from the Local<Context>, which we receive during module registration. So, basically, the first N-API module that is loaded creates then env against the Local<Context> it receives, and stores it on the global it gets from Context::Global(). Subsequent N-API modules just retrieve this napi_env from the global.

@addaleax
Copy link
Member Author

addaleax commented Sep 14, 2018

a napi_env is wherein all napi_values live. That is, if I have two napi_envs and I try to manipulate a napi_value coming from one napi_env with the other napi_env It'll crash.

That would map to a V8 Isolate/JS engine instance. However, N-API also now provides access to some event-loop based mechanisms, which don’t need to relate to those in some specific form, which makes this a bit more complex?

In practice, we store the napi_env on the global object, so it's one napi_env per global object. We retrieve the global object from the Local<Context>, which we receive during module registration. So, basically, the first N-API module that is loaded creates then env against the Local<Context> it receives, and stores it on the global it gets from Context::Global(). Subsequent N-API modules just retrieve this napi_env from the global.

That’s on another level as separation based on JS engine instances, though – there can be multiple Contexts/global objects per instance? I’ll be removing the TODO comment here, though, it doesn’t seem necessary according to this.

@addaleax
Copy link
Member Author

addaleax commented Sep 17, 2018

Landed in 4286dcf

@addaleax addaleax closed this Sep 17, 2018
@addaleax addaleax deleted the env-get-current branch September 17, 2018 15:25
SirR4T pushed a commit to SirR4T/node that referenced this pull request Sep 17, 2018
@addaleax
src: refactor Environment::GetCurrent() usage
4286dcf 
Make `Environment::GetCurrent()` return `nullptr` if the current
`Context` is not a Node.js context, and for the relevant usage of
this function, either:

- Switch to the better `GetCurrent(args)` variant
- Turn functions in to no-ops where it makes sense
- Make it a `CHECK`, i.e. an API requirement, where it make sense
- Leave a `TODO` comment for verifying what, if anything, is to be done

PR-URL: nodejs#22819
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 18, 2018
@addaleax @targos
src: refactor Environment::GetCurrent() usage
2e32a75 
Make `Environment::GetCurrent()` return `nullptr` if the current
`Context` is not a Node.js context, and for the relevant usage of
this function, either:

- Switch to the better `GetCurrent(args)` variant
- Turn functions in to no-ops where it makes sense
- Make it a `CHECK`, i.e. an API requirement, where it make sense
- Leave a `TODO` comment for verifying what, if anything, is to be done

PR-URL: #22819
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos mentioned this pull request Sep 18, 2018
Merged
targos pushed a commit that referenced this pull request Sep 19, 2018
@addaleax @targos
src: refactor Environment::GetCurrent() usage
721508d 
Make `Environment::GetCurrent()` return `nullptr` if the current
`Context` is not a Node.js context, and for the relevant usage of
this function, either:

- Switch to the better `GetCurrent(args)` variant
- Turn functions in to no-ops where it makes sense
- Make it a `CHECK`, i.e. an API requirement, where it make sense
- Leave a `TODO` comment for verifying what, if anything, is to be done

PR-URL: #22819
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2018
@addaleax @targos
src: refactor Environment::GetCurrent() usage
5d5c3fa 
Make `Environment::GetCurrent()` return `nullptr` if the current
`Context` is not a Node.js context, and for the relevant usage of
this function, either:

- Switch to the better `GetCurrent(args)` variant
- Turn functions in to no-ops where it makes sense
- Make it a `CHECK`, i.e. an API requirement, where it make sense
- Leave a `TODO` comment for verifying what, if anything, is to be done

PR-URL: #22819
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+3 more reviewers

@refack refack refack approved these changes

@eugeneo eugeneo eugeneo approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@addaleax @nodejs-github-bot @mhdawson @gabrielschulhof @refack @eugeneo @devsnek @gireeshpunathil